Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
📝 WalkthroughWalkthroughThe PR adds offline delta sync for incident command and ad-hoc resources, plus idempotent replay handling for check-in requests. It also adds phone-number validation, SMS content filtering, widened address validation, and a portable database time query. ChangesOffline Sync Change Tracking
Check-in Replay Idempotency
Phone, SMS, and address validation
Infrastructure and health
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
Providers/Resgrid.Providers.MigrationsPg/Migrations/M0081_AddIncidentCommandChangeTrackingPg.cs (1)
25-40: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSame
NULLModifiedOnbackfill gap as the SQL Server migration.This PostgreSQL migration mirrors
M0081_AddIncidentCommandChangeTrackingand shares the same issue: existing rows receiveNULLModifiedOnand will be excluded from the initial delta/full pull. Apply the same backfill here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0081_AddIncidentCommandChangeTrackingPg.cs` around lines 25 - 40, The PostgreSQL migration Up method in M0081_AddIncidentCommandChangeTrackingPg adds ModifiedOn as nullable but does not backfill existing rows, so they stay NULL and miss the initial sync. Update the migration to set ModifiedOn for all pre-existing rows in each table from ChangeTrackedTables, using the same backfill approach as the SQL Server migration before or after adding the column. Keep the existing CommandStructureNodes DeletedOn addition unchanged, and make sure the backfill is applied consistently within Up.
🧹 Nitpick comments (2)
Providers/Resgrid.Providers.Migrations/Migrations/M0082_AddCheckInRecordIdempotencyKey.cs (1)
12-18: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftConsider a partial unique index to enforce idempotency at the DB level.
A plain nullable column does not prevent duplicate inserts under concurrent replay (see
PerformCheckInAsync). A filtered unique index onIdempotencyKey(scoped per department, where non-null) would make the dedup guarantee robust and let the service catch the violation deterministically.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Providers/Resgrid.Providers.Migrations/Migrations/M0082_AddCheckInRecordIdempotencyKey.cs` around lines 12 - 18, Add a DB-level idempotency guard in M0082_AddCheckInRecordIdempotencyKey instead of only adding the nullable IdempotencyKey column. Update the migration to create a filtered unique index on CheckInRecords.IdempotencyKey, scoped with DepartmentId (or the same tenant scope used by PerformCheckInAsync) and excluding null values, so concurrent replays cannot insert duplicates. Make sure the index is created alongside the existing AddColumn change and that the service layer can deterministically handle the unique-constraint violation.Core/Resgrid.Services/IncidentCommandService.cs (1)
365-396: 🚀 Performance & Scalability | 🔵 TrivialDelta read fans out to 8 full department-scoped loads + in-memory filtering on every poll.
Each
GetChangesSinceAsynccall pulls every command/node/assignment/objective/timer/annotation/role/timeline row for the department and filters in memory. Since soft-deleted/closed/released rows are intentionally never purged, these sets grow unbounded over the life of the department, so polling clients pay an ever-increasing cost. The class header already flags per-CallId/predicate repository queries as a follow-up — consider aModifiedOn >@since`` (indexed) repository query before this ships to high-traffic departments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Services/IncidentCommandService.cs` around lines 365 - 396, GetChangesSinceAsync in IncidentCommandService is loading entire department-scoped collections and filtering them in memory, which will scale poorly as data grows. Update the repository calls for commands, nodes, assignments, objectives, timers, annotations, roles, and timeline entries to query by sinceUtc/ModifiedOn (or OccurredOn for timeline) at the repository level instead of using GetAllByDepartmentIdAsync plus Changed filtering. Keep the existing change-shaping logic, but move the predicate into the repository methods or add new indexed query methods so the method only returns delta rows.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Core/Resgrid.Model/IncidentCommand/IncidentCommandChanges.cs`:
- Around line 10-11: The XML doc comment on IncidentCommandChanges contradicts
the actual sync behavior exposed by AdHocUnits, AdHocPersonnel, and
GetAdHocChangesSinceAsync. Update the remark to describe these ad-hoc resources
as delta-tracked by ModifiedOn and returned through the incremental changes
payload, rather than saying they are not change-tracked or require a full
refetch. Keep the wording aligned with the model fields and the filtering logic
so clients can rely on the sync contract.
In `@Core/Resgrid.Services/CheckInTimerService.cs`:
- Around line 326-332: The idempotency guard in CheckInTimerService is only
doing a read-before-write check, so concurrent replays can still insert
duplicates. Add a filtered unique index for the check-in record idempotency key
on DepartmentId and IdempotencyKey with IdempotencyKey IS NOT NULL in the
M0082_AddCheckInRecordIdempotencyKey migration, then update the
CheckInTimerService insert flow to catch the unique-violation and return the
existing record instead of failing.
In `@Core/Resgrid.Services/IncidentCommandService.cs`:
- Around line 363-396: The delta-pull logic in IncidentCommandService currently
filters everything through the Changed(IChangeTracked) predicate, so rows with
null ModifiedOn are skipped even during the initial full sync. Update this flow
so the sinceUtc initial-cursor case (DateTime.MinValue) returns all rows or
otherwise includes legacy null-ModifiedOn records, and keep the filtering
behavior for normal incremental syncs; use the existing Changed helper and the
repository fetch blocks for commands, nodes, assignments, objectives, timers,
annotations, and roles to apply the fix consistently.
In
`@Providers/Resgrid.Providers.Migrations/Migrations/M0081_AddIncidentCommandChangeTracking.cs`:
- Around line 26-40: Backfill the new ModifiedOn column in
M0081_AddIncidentCommandChangeTracking so existing rows are visible to
GetChangesSinceAsync on the first sync. Update the Up() migration logic around
ChangeTrackedTables and the CommandStructureNodes alter to seed ModifiedOn for
pre-existing records, not just add it as nullable, so current
commands/nodes/assignments/objectives/timers/annotations/roles are included in
the initial delta pull.
In
`@Providers/Resgrid.Providers.Migrations/Migrations/M0083_AddAdHocResourceChangeTracking.cs`:
- Around line 14-23: The migration that adds `ModifiedOn` to the ad-hoc tables
leaves existing rows with null timestamps, so they never satisfy the
`GetAdHocChangesSinceAsync` change filter and won’t sync on reconnect or first
pull. Update `M0083_AddAdHocResourceChangeTracking.Up()` to backfill
`ModifiedOn` for pre-existing rows in the affected tables (`IncidentAdHocUnits`
and `IncidentAdHocPersonnel`) after adding the column, using an appropriate
existing timestamp source such as `CreatedOn`, `ReleasedOn`, or the current UTC
time so legacy records participate in delta sync.
In
`@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0083_AddAdHocResourceChangeTrackingPg.cs`:
- Around line 19-21: The M0083_AddAdHocResourceChangeTrackingPg migration adds
the nullable ModifiedOn column in the table loop but does not populate it for
existing records, so pre-existing rows will remain NULL and be skipped by
/Sync/Changes. Update the migration logic around the Schema.Table(t).Exists()
and Alter.Table(t).AddColumn("ModifiedOn") flow to backfill a sensible timestamp
for existing rows after adding the column, using the same migration path and
table iteration that already handles the column creation.
---
Duplicate comments:
In
`@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0081_AddIncidentCommandChangeTrackingPg.cs`:
- Around line 25-40: The PostgreSQL migration Up method in
M0081_AddIncidentCommandChangeTrackingPg adds ModifiedOn as nullable but does
not backfill existing rows, so they stay NULL and miss the initial sync. Update
the migration to set ModifiedOn for all pre-existing rows in each table from
ChangeTrackedTables, using the same backfill approach as the SQL Server
migration before or after adding the column. Keep the existing
CommandStructureNodes DeletedOn addition unchanged, and make sure the backfill
is applied consistently within Up.
---
Nitpick comments:
In `@Core/Resgrid.Services/IncidentCommandService.cs`:
- Around line 365-396: GetChangesSinceAsync in IncidentCommandService is loading
entire department-scoped collections and filtering them in memory, which will
scale poorly as data grows. Update the repository calls for commands, nodes,
assignments, objectives, timers, annotations, roles, and timeline entries to
query by sinceUtc/ModifiedOn (or OccurredOn for timeline) at the repository
level instead of using GetAllByDepartmentIdAsync plus Changed filtering. Keep
the existing change-shaping logic, but move the predicate into the repository
methods or add new indexed query methods so the method only returns delta rows.
In
`@Providers/Resgrid.Providers.Migrations/Migrations/M0082_AddCheckInRecordIdempotencyKey.cs`:
- Around line 12-18: Add a DB-level idempotency guard in
M0082_AddCheckInRecordIdempotencyKey instead of only adding the nullable
IdempotencyKey column. Update the migration to create a filtered unique index on
CheckInRecords.IdempotencyKey, scoped with DepartmentId (or the same tenant
scope used by PerformCheckInAsync) and excluding null values, so concurrent
replays cannot insert duplicates. Make sure the index is created alongside the
existing AddColumn change and that the service layer can deterministically
handle the unique-constraint violation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3951bf55-d737-4e8a-a00d-815e42c39750
⛔ Files ignored due to path filters (5)
Tests/Resgrid.Tests/Services/CheckInTimerServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/IncidentCommandServiceParTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/IncidentResourcesServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/IncidentVoiceServiceTests.csis excluded by!**/Tests/**docs/architecture/offline-first-architecture.mdis excluded by!**/*.md
📒 Files selected for processing (25)
Core/Resgrid.Model/CheckInRecord.csCore/Resgrid.Model/IChangeTracked.csCore/Resgrid.Model/IncidentCommand/CommandStructureNode.csCore/Resgrid.Model/IncidentCommand/IncidentAdHocResources.csCore/Resgrid.Model/IncidentCommand/IncidentCommand.csCore/Resgrid.Model/IncidentCommand/IncidentCommandChanges.csCore/Resgrid.Model/IncidentCommand/IncidentRole.csCore/Resgrid.Model/IncidentCommand/IncidentTacticals.csCore/Resgrid.Model/Services/IIncidentCommandService.csCore/Resgrid.Model/Services/IIncidentResourcesService.csCore/Resgrid.Services/CheckInTimerService.csCore/Resgrid.Services/IncidentCommandService.csCore/Resgrid.Services/IncidentResourcesService.csCore/Resgrid.Services/IncidentVoiceService.csProviders/Resgrid.Providers.Migrations/Migrations/M0081_AddIncidentCommandChangeTracking.csProviders/Resgrid.Providers.Migrations/Migrations/M0082_AddCheckInRecordIdempotencyKey.csProviders/Resgrid.Providers.Migrations/Migrations/M0083_AddAdHocResourceChangeTracking.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0081_AddIncidentCommandChangeTrackingPg.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0082_AddCheckInRecordIdempotencyKeyPg.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0083_AddAdHocResourceChangeTrackingPg.csWeb/Resgrid.Web.Services/Controllers/v4/CheckInTimersController.csWeb/Resgrid.Web.Services/Controllers/v4/SyncController.csWeb/Resgrid.Web.Services/Models/v4/CheckInTimers/CheckInTimerModels.csWeb/Resgrid.Web.Services/Models/v4/Sync/SyncModels.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xml
Comment on lines
+10
to
+11
| /// Ad-hoc resources are not change-tracked and are pulled separately (full refetch). See | ||
| /// docs/architecture/offline-first-architecture.md. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Doc comment contradicts the model and the actual delta behavior.
The remark says ad-hoc resources "are not change-tracked and are pulled separately (full refetch)", yet this payload exposes AdHocUnits/AdHocPersonnel (Lines 32-34) and GetAdHocChangesSinceAsync filters them by ModifiedOn (delta, not full refetch). This will mislead clients implementing the sync contract.
📝 Suggested doc fix
- /// Soft-deleted / closed / released rows ARE included (with their state columns set) so the client can remove or
- /// update them locally. The client stores <see cref="ServerTimestampMs"/> and passes it back as the next `since`.
- /// Ad-hoc resources are not change-tracked and are pulled separately (full refetch). See
- /// docs/architecture/offline-first-architecture.md.
+ /// Soft-deleted / closed / released rows ARE included (with their state columns set) so the client can remove or
+ /// update them locally. The client stores <see cref="ServerTimestampMs"/> and passes it back as the next `since`.
+ /// Ad-hoc resources are change-tracked via ModifiedOn and delta-pulled alongside command rows (aggregated by
+ /// SyncController). See docs/architecture/offline-first-architecture.md.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Ad-hoc resources are not change-tracked and are pulled separately (full refetch). See | |
| /// docs/architecture/offline-first-architecture.md. | |
| /// Soft-deleted / closed / released rows ARE included (with their state columns set) so the client can remove or | |
| /// update them locally. The client stores <see cref="ServerTimestampMs"/> and passes it back as the next `since`. | |
| /// Ad-hoc resources are change-tracked via ModifiedOn and delta-pulled alongside command rows (aggregated by | |
| /// SyncController). See docs/architecture/offline-first-architecture.md. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Core/Resgrid.Model/IncidentCommand/IncidentCommandChanges.cs` around lines 10
- 11, The XML doc comment on IncidentCommandChanges contradicts the actual sync
behavior exposed by AdHocUnits, AdHocPersonnel, and GetAdHocChangesSinceAsync.
Update the remark to describe these ad-hoc resources as delta-tracked by
ModifiedOn and returned through the incremental changes payload, rather than
saying they are not change-tracked or require a full refetch. Keep the wording
aligned with the model fields and the filtering logic so clients can rely on the
sync contract.
Comment on lines
+26
to
+40
| public override void Up() | ||
| { | ||
| foreach (var table in ChangeTrackedTables) | ||
| { | ||
| if (Schema.Table(table).Exists() && !Schema.Table(table).Column("ModifiedOn").Exists()) | ||
| { | ||
| Alter.Table(table).AddColumn("ModifiedOn").AsDateTime2().Nullable(); | ||
| } | ||
| } | ||
|
|
||
| if (Schema.Table("CommandStructureNodes").Exists() && !Schema.Table("CommandStructureNodes").Column("DeletedOn").Exists()) | ||
| { | ||
| Alter.Table("CommandStructureNodes").AddColumn("DeletedOn").AsDateTime2().Nullable(); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the command-side delta reader to see how null ModifiedOn is handled
fd -t f 'IncidentCommandService.cs' Core/Resgrid.Services --exec sed -n '320,400p' {}Repository: Resgrid/Core
Length of output: 3732
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the sync controller cursor handling and the migration context
fd -t f 'SyncController.cs' Core/Resgrid.* --exec sed -n '1,240p' {}
printf '\n---\n'
fd -t f 'M0081_AddIncidentCommandChangeTracking.cs' Providers/Resgrid.Providers.Migrations --exec sed -n '1,220p' {}Repository: Resgrid/Core
Length of output: 2018
Backfill ModifiedOn for existing rows in M0081_AddIncidentCommandChangeTracking.cs.
GetChangesSinceAsync only surfaces rows with ModifiedOn.HasValue && ModifiedOn > sinceUtc, so adding the column as nullable leaves all pre-existing commands/nodes/assignments/objectives/timers/annotations/roles invisible to the initial delta pull. Seed the new column during Up() so current board state is included on first sync.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Providers/Resgrid.Providers.Migrations/Migrations/M0081_AddIncidentCommandChangeTracking.cs`
around lines 26 - 40, Backfill the new ModifiedOn column in
M0081_AddIncidentCommandChangeTracking so existing rows are visible to
GetChangesSinceAsync on the first sync. Update the Up() migration logic around
ChangeTrackedTables and the CommandStructureNodes alter to seed ModifiedOn for
pre-existing records, not just add it as nullable, so current
commands/nodes/assignments/objectives/timers/annotations/roles are included in
the initial delta pull.
Comment on lines
+14
to
+23
| public override void Up() | ||
| { | ||
| foreach (var table in Tables) | ||
| { | ||
| if (Schema.Table(table).Exists() && !Schema.Table(table).Column("ModifiedOn").Exists()) | ||
| { | ||
| Alter.Table(table).AddColumn("ModifiedOn").AsDateTime2().Nullable(); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Pre-existing ad-hoc rows will never appear in delta sync.
ModifiedOn is added as nullable with no backfill, so any IncidentAdHocUnits/IncidentAdHocPersonnel rows created before this migration keep ModifiedOn = NULL. The downstream Changed predicate in GetAdHocChangesSinceAsync requires e.ModifiedOn.HasValue && e.ModifiedOn.Value > sinceUtc, so those rows are excluded even on an initial full pull (since <= 0 → DateTime.MinValue). A reconnecting client would silently miss all pre-migration ad-hoc resources until they are touched again.
Consider backfilling ModifiedOn (e.g. from CreatedOn/ReleasedOn, or GETUTCDATE()) in Up() so existing rows participate in the first sync.
🛠️ Possible backfill
if (Schema.Table(table).Exists() && !Schema.Table(table).Column("ModifiedOn").Exists())
{
Alter.Table(table).AddColumn("ModifiedOn").AsDateTime2().Nullable();
+ Execute.Sql($"UPDATE [{table}] SET [ModifiedOn] = COALESCE([ReleasedOn], [CreatedOn]) WHERE [ModifiedOn] IS NULL");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public override void Up() | |
| { | |
| foreach (var table in Tables) | |
| { | |
| if (Schema.Table(table).Exists() && !Schema.Table(table).Column("ModifiedOn").Exists()) | |
| { | |
| Alter.Table(table).AddColumn("ModifiedOn").AsDateTime2().Nullable(); | |
| } | |
| } | |
| } | |
| public override void Up() | |
| { | |
| foreach (var table in Tables) | |
| { | |
| if (Schema.Table(table).Exists() && !Schema.Table(table).Column("ModifiedOn").Exists()) | |
| { | |
| Alter.Table(table).AddColumn("ModifiedOn").AsDateTime2().Nullable(); | |
| Execute.Sql($"UPDATE [{table}] SET [ModifiedOn] = COALESCE([ReleasedOn], [CreatedOn]) WHERE [ModifiedOn] IS NULL"); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Providers/Resgrid.Providers.Migrations/Migrations/M0083_AddAdHocResourceChangeTracking.cs`
around lines 14 - 23, The migration that adds `ModifiedOn` to the ad-hoc tables
leaves existing rows with null timestamps, so they never satisfy the
`GetAdHocChangesSinceAsync` change filter and won’t sync on reconnect or first
pull. Update `M0083_AddAdHocResourceChangeTracking.Up()` to backfill
`ModifiedOn` for pre-existing rows in the affected tables (`IncidentAdHocUnits`
and `IncidentAdHocPersonnel`) after adding the column, using an appropriate
existing timestamp source such as `CreatedOn`, `ReleasedOn`, or the current UTC
time so legacy records participate in delta sync.
Comment on lines
+19
to
+21
| if (Schema.Table(t).Exists() && !Schema.Table(t).Column("ModifiedOn".ToLower()).Exists()) | ||
| { | ||
| Alter.Table(t).AddColumn("ModifiedOn".ToLower()).AsDateTime2().Nullable(); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor
🧩 Analysis chain
🌐 Web query:
FluentMigrator 7.0.0 Postgres generator AsDateTime2 column type mapping
💡 Result:
In FluentMigrator 7.0.0, the AsDateTime2() column type extension is supported and maps to the timestamp data type when using the PostgreSQL generator [1][2]. While AsDateTime2() is explicitly available in the FluentMigrator API [1][3], the PostgreSQL generator maps DbType.DateTime2 to the standard PostgreSQL timestamp type [2]. This behavior is consistent across recent versions of FluentMigrator, as PostgreSQL's timestamp type effectively covers the functional requirements typically associated with DateTime2 in other SQL dialects [4][2]. If you require specific PostgreSQL timestamp behaviors—such as time zone awareness—it is recommended to use AsDateTimeOffset(), which maps to the PostgreSQL timestamptz (timestamp with time zone) type [2]. For scenarios requiring database-specific precision or custom timestamp definitions that the built-in generator does not provide, you can use the AsCustom("type_name") method to explicitly define the column type in your migration [1][5]. Top results: [1], [3], [2]
Citations:
- 1: https://fluentmigrator.github.io/basics/columns.html
- 2: https://github.com/fluentmigrator/fluentmigrator/blob/master/src/FluentMigrator.Runner.Postgres/Generators/Postgres/PostgresTypeMap.cs
- 3: https://fluentmigrator.github.io/operations/create-tables.html
- 4: Support datetime2 type fluentmigrator/fluentmigrator#974
- 5: https://fluentmigrator.github.io/advanced/custom-extensions.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target migration and the related SQL Server migration for the same version.
git ls-files 'Providers/Resgrid.Providers.MigrationsPg/Migrations/M0083_AddAdHocResourceChangeTrackingPg.cs' \
'Providers/Resgrid.Providers.MigrationsSql/Migrations/M0083_AddAdHocResourceChangeTracking.cs' \
'Providers/Resgrid.Providers.Migrations*/Migrations/M0083_AddAdHocResourceChangeTracking*.cs'
echo '--- Pg migration outline ---'
ast-grep outline Providers/Resgrid.Providers.MigrationsPg/Migrations/M0083_AddAdHocResourceChangeTrackingPg.cs --view expanded || true
echo '--- Sql migration outline ---'
ast-grep outline Providers/Resgrid.Providers.MigrationsSql/Migrations/M0083_AddAdHocResourceChangeTracking.cs --view expanded || true
echo '--- Pg migration lines ---'
sed -n '1,220p' Providers/Resgrid.Providers.MigrationsPg/Migrations/M0083_AddAdHocResourceChangeTrackingPg.cs
echo '--- Sql migration lines ---'
sed -n '1,240p' Providers/Resgrid.Providers.MigrationsSql/Migrations/M0083_AddAdHocResourceChangeTracking.csRepository: Resgrid/Core
Length of output: 1923
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If the migrations differ, check whether the Pg migration also backfills the new column.
rg -n "ModifiedOn|Update.*ModifiedOn|Set.*ModifiedOn|Backfill|Execute\.Sql|Update\(" Providers/Resgrid.Providers.MigrationsPg/Migrations/M0083_AddAdHocResourceChangeTrackingPg.cs Providers/Resgrid.Providers.MigrationsSql/Migrations/M0083_AddAdHocResourceChangeTracking.csRepository: Resgrid/Core
Length of output: 1203
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the shared migration counterpart to see whether it performs a backfill or uses a different type.
sed -n '1,240p' Providers/Resgrid.Providers.Migrations/Migrations/M0083_AddAdHocResourceChangeTracking.csRepository: Resgrid/Core
Length of output: 1174
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the PostgreSQL generator maps DateTime2 to timestamp in the bundled FluentMigrator sources, if present.
rg -n "DateTime2|timestamp" -g '*/PostgresTypeMap.cs' -g '*/Postgres*.cs' .Repository: Resgrid/Core
Length of output: 150
Add a backfill for existing rows
Providers/Resgrid.Providers.MigrationsPg/Migrations/M0083_AddAdHocResourceChangeTrackingPg.cs:19-21 adds a nullable ModifiedOn column, but leaves existing rows NULL. Seed the column for pre-existing records if they should participate in /Sync/Changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0083_AddAdHocResourceChangeTrackingPg.cs`
around lines 19 - 21, The M0083_AddAdHocResourceChangeTrackingPg migration adds
the nullable ModifiedOn column in the table loop but does not populate it for
existing records, so pre-existing rows will remain NULL and be skipped by
/Sync/Changes. Update the migration logic around the Schema.Table(t).Exists()
and Alter.Table(t).AddColumn("ModifiedOn") flow to backfill a sensible timestamp
for existing rows after adding the column, using the same migration path and
table iteration that already handles the column creation.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Core/Resgrid.Services/CheckInTimerService.cs`:
- Around line 341-350: The replay-adoption catch in CheckInTimerService’s save
flow is too broad because it catches every exception and can mask real failures
as successful idempotent replays. Narrow the exception filter around the
SaveOrUpdateAsync path to only handle PostgreSQL unique-constraint violations by
catching Npgsql.PostgresException and checking SqlState == "23505" together with
a non-empty record.IdempotencyKey; keep the existing winner lookup logic, but
let all other exceptions propagate immediately so non-unique-key errors are not
swallowed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7a28cb24-6850-4249-bcd8-4a404bd2208e
⛔ Files ignored due to path filters (3)
Tests/Resgrid.Tests/Services/CheckInTimerServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/IncidentCommandServiceParTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/IncidentResourcesServiceTests.csis excluded by!**/Tests/**
📒 Files selected for processing (5)
Core/Resgrid.Services/CheckInTimerService.csCore/Resgrid.Services/IncidentCommandService.csCore/Resgrid.Services/IncidentResourcesService.csProviders/Resgrid.Providers.Migrations/Migrations/M0082_AddCheckInRecordIdempotencyKey.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0082_AddCheckInRecordIdempotencyKeyPg.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- Providers/Resgrid.Providers.MigrationsPg/Migrations/M0082_AddCheckInRecordIdempotencyKeyPg.cs
- Core/Resgrid.Services/IncidentResourcesService.cs
- Core/Resgrid.Services/IncidentCommandService.cs
Comment on lines
+341
to
+350
| catch (Exception) when (!string.IsNullOrWhiteSpace(record.IdempotencyKey)) | ||
| { | ||
| // The in-memory pre-check above is check-then-insert and races under concurrent replays; the filtered | ||
| // unique index UX_CheckInRecords_Department_IdempotencyKey is the real guard. If we lost the race, | ||
| // adopt the winner — same idempotent result as the pre-check — rather than surfacing a 500 (which would | ||
| // just trigger another retry). | ||
| var existing = await _recordRepository.GetByCallIdAsync(record.CallId); | ||
| var winner = existing?.FirstOrDefault(r => r.IdempotencyKey == record.IdempotencyKey); | ||
| if (winner != null) | ||
| return winner; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify which provider/repository exception types represent unique constraint violations
# so the catch filter can be narrowed without guessing.
rg -nP -C3 '(SaveOrUpdateAsync|SqlException|PostgresException|DbUpdateException|unique constraint|duplicate key|2601|2627|23505)' Core Providers -g '*.cs'Repository: Resgrid/Core
Length of output: 50369
🏁 Script executed:
python3 - <<'PY'
code_snippet = """
try
{
saved = await _recordRepository.SaveOrUpdateAsync(record, cancellationToken);
}
catch (Exception) when (!string.IsNullOrWhiteSpace(record.IdempotencyKey))
{
var existing = await _recordRepository.GetByCallIdAsync(record.CallId);
var winner = existing?.FirstOrDefault(r => r.IdempotencyKey == record.IdempotencyKey);
if (winner != null)
return winner;
throw;
}
"""
# Analyze the behavior of `throw` in this specific context
analysis = """
The snippet uses `catch (Exception)` without specifying a variable name.
Inside the block:
1. `var existing = await ...` is an I/O call. If this call fails, an *new* exception is awaited.
- If it throws an `OperationCanceledException`, that exception bubbles up immediately. The original `Exception` is effectively lost/swapped for the new failure if the await fails, or the new one propagates.
- If it succeeds but `winner` is null, the code hits `throw;`.
2. C# `throw;` with no arguments re-throws the *original* exception caught by the `catch` block (the one from `SaveOrUpdateAsync`).
3. Therefore, if `winner` is null, the original exception propagates correctly.
However, the risk remains if `SaveOrUpdateAsync` throws an exception that is NOT a unique violation (e.g., FK constraint violation "23503", Serialization error, or Timeout), and `existing` query *successfully* returns a record (perhaps from a different idempotency key or just a stale read).
If `winner` is found (even if it's the wrong idempotency key? No, the code checks `r.IdempotencyKey == record.IdempotencyKey`), then it returns the winner.
The logic `if (winner != null)` only returns if a record matches the key.
If the original exception was a Timeout, but `winner` happens to exist in the DB with the matching key (from a previous retry that partially completed), the current code treats it as a success.
CRITICAL ISSUE: The exception filter `when (!string.IsNullOrWhiteSpace(record.IdempotencyKey))` applies to the ENTRY of the catch block.
It does NOT guard against:
1. Non-unique exceptions (like Foreign Keys) where a matching record happens to exist.
2. It is safer to check the exception type explicitly.
"""
print(analysis)
PYRepository: Resgrid/Core
Length of output: 1853
Limit retry adoption strictly to unique-key violations.
The current retry logic catches all exceptions regardless of type, risking the masking of valid failures (e.g., timeouts, foreign key violations) as successful replays if a matching row happens to exist. For example, if a transaction times out but a partial write created the record, the code returns success even though the original operation failed with a non-unique-key error.
Refine the exception handling to filter only for PostgreSQL unique constraint violations (SqlState == "23505") to ensure other errors propagate immediately.
Proposed Fix
try
{
saved = await _recordRepository.SaveOrUpdateAsync(record, cancellationToken);
}
catch (Npgsql.PostgresException ex) when (ex.SqlState == "23505" && !string.IsNullOrWhiteSpace(record.IdempotencyKey))
{
var existing = await _recordRepository.GetByCallIdAsync(record.CallId, cancellationToken);
var winner = existing?.FirstOrDefault(r => r.IdempotencyKey == record.IdempotencyKey);
if (winner != null)
return winner;
throw;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Core/Resgrid.Services/CheckInTimerService.cs` around lines 341 - 350, The
replay-adoption catch in CheckInTimerService’s save flow is too broad because it
catches every exception and can mask real failures as successful idempotent
replays. Narrow the exception filter around the SaveOrUpdateAsync path to only
handle PostgreSQL unique-constraint violations by catching
Npgsql.PostgresException and checking SqlState == "23505" together with a
non-empty record.IdempotencyKey; keep the existing winner lookup logic, but let
all other exceptions propagate immediately so non-unique-key errors are not
swallowed.
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
Web/Resgrid.Web/wwwroot/js/app/resgrid.phonevalidate.js (1)
43-43: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRender server values via
textContentinstead ofinnerHTMLconcatenation.
data.formatted(and the capturedvalue) are interpolated directly intoinnerHTMLat Lines 43, 55, and 68. While the value currently comes from the server-side phone parser, building these nodes withtextContenteliminates the XSS class flagged by static analysis and is more robust against future endpoint changes.🛡️ Example hardening for the "Use this" button
btn.addEventListener('click', function () { input.value = value; - feedback.innerHTML = '<span class="text-success">Updated to ' + value + '</span>'; + feedback.innerHTML = ''; + var span = document.createElement('span'); + span.className = 'text-success'; + span.textContent = 'Updated to ' + value; + feedback.appendChild(span); });Apply the same node-building approach for the
ok/sugblocks (Lines 55, 68), usingtextContentfor the formatted value inside the<strong>element.Also applies to: 48-72
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/wwwroot/js/app/resgrid.phonevalidate.js` at line 43, The phone validation feedback in resgrid.phonevalidate.js is building status messages with innerHTML and string concatenation, which should be hardened to avoid injecting server-derived values. Update the handlers in the feedback/update, ok, and sug blocks to construct the DOM nodes explicitly and assign the formatted/value text via textContent instead of interpolating data.formatted or value into HTML. Keep the existing UI structure and class names, but ensure the message text is inserted only as text in the relevant elements.Source: Linters/SAST tools
Providers/Resgrid.Providers.Migrations/Migrations/M0085_WidenAddressColumns.cs (1)
19-19: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
Address1asnvarchar(max)is broader than needed and conflicts with the model cap.
Address.Address1now validates atStringLength(500), yet this widens the DB column toInt32.MaxValue(nvarchar(max)). A bounded type (e.g.AsString(500)or a generous fixed cap) keeps DB and model consistent, remains indexable, and avoids the storage/perf overhead ofMAX.♻️ Suggested change
- Alter.Table("Addresses").AlterColumn("Address1").AsString(Int32.MaxValue).NotNullable(); + Alter.Table("Addresses").AlterColumn("Address1").AsString(500).NotNullable();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Providers/Resgrid.Providers.Migrations/Migrations/M0085_WidenAddressColumns.cs` at line 19, The M0085_WidenAddressColumns migration is widening Addresses.Address1 to nvarchar(max), which does not match the model’s StringLength(500) cap. Update the Alter.Table("Addresses").AlterColumn("Address1") definition to use a bounded string length that aligns with Address.Address1 validation (for example, 500 or a similarly fixed cap) so the database schema stays consistent with the model and avoids MAX storage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Providers/Resgrid.Providers.Migrations/Migrations/M0085_WidenAddressColumns.cs`:
- Around line 20-23: The migration in M0085_WidenAddressColumns is tightening
address fields to NotNullable() without preparing existing data, which can fail
on legacy rows. Update the migration flow around the
Alter.Table("Addresses").AlterColumn(...) calls to first backfill or default
missing values for Address1, City, State, and Country, or keep the columns
nullable if the model still permits nulls. Use the migration class
M0085_WidenAddressColumns and its address column alterations to place the
data-fix step before enforcing NOT NULL.
In `@Repositories/Resgrid.Repositories.DataRepository/HealthRepository.cs`:
- Around line 26-33: The HealthRepository connectivity check is swallowing all
exceptions and returning null, which hides real DB failures and mapping issues.
In HealthRepository’s method that wraps the db.QueryAsync<DateTime>(query) call,
catch the exception, call Resgrid.Framework.Logging.LogException(ex, ...) with
enough context to identify the health check operation, and then keep the
existing null fallback behavior. Use the existing HealthRepository method and
the _connectionProvider/Create/db.QueryAsync flow as the place to add the
logging so failures become diagnosable.
In `@Web/Resgrid.Web/Areas/User/Models/Contacts/EditContactView.cs`:
- Around line 46-59: The edit POST is mapping the mailing address from the wrong
source fields, causing mailing changes to be ignored and potentially overwritten
by the physical address. In ContactsController’s edit/save flow, update the
mailing Address assignment to use the model’s Mailing* properties instead of
Physical* values, and keep the existing EditContactView validation as-is. Use
the ContactsController mailing-address mapping block and the EditContactView
mailing fields to locate and correct the assignment.
In `@Web/Resgrid.Web/Areas/User/Views/Personnel/AddPerson.cshtml`:
- Line 116: The AddPerson view is missing the standard client-side validation
hookup for the new Profile.MobileNumber validation span. Update the Scripts
section in AddPerson.cshtml to include _ValidationScriptsPartial alongside
resgrid.phonevalidate.js so asp-validation-for="Profile.MobileNumber" renders
unobtrusive validation consistently, matching the behavior in
Contacts/EditUserProfile.
---
Nitpick comments:
In
`@Providers/Resgrid.Providers.Migrations/Migrations/M0085_WidenAddressColumns.cs`:
- Line 19: The M0085_WidenAddressColumns migration is widening
Addresses.Address1 to nvarchar(max), which does not match the model’s
StringLength(500) cap. Update the
Alter.Table("Addresses").AlterColumn("Address1") definition to use a bounded
string length that aligns with Address.Address1 validation (for example, 500 or
a similarly fixed cap) so the database schema stays consistent with the model
and avoids MAX storage.
In `@Web/Resgrid.Web/wwwroot/js/app/resgrid.phonevalidate.js`:
- Line 43: The phone validation feedback in resgrid.phonevalidate.js is building
status messages with innerHTML and string concatenation, which should be
hardened to avoid injecting server-derived values. Update the handlers in the
feedback/update, ok, and sug blocks to construct the DOM nodes explicitly and
assign the formatted/value text via textContent instead of interpolating
data.formatted or value into HTML. Keep the existing UI structure and class
names, but ensure the message text is inserted only as text in the relevant
elements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: adf21f5c-ac1d-4357-b82d-ac558fc34507
⛔ Files ignored due to path filters (4)
Tests/Resgrid.Tests/Services/CheckInTimerServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/ContactVerificationServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/PhoneRegionHelperTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/SmsContentHelperTests.csis excluded by!**/Tests/**
📒 Files selected for processing (32)
Core/Resgrid.Config/SystemBehaviorConfig.csCore/Resgrid.Framework/PhoneRegionHelper.csCore/Resgrid.Framework/SmsContentHelper.csCore/Resgrid.Model/Address.csCore/Resgrid.Services/CheckInTimerService.csCore/Resgrid.Services/ContactVerificationService.csProviders/Resgrid.Providers.Migrations/Migrations/M0084_WidenAutofillDataColumn.csProviders/Resgrid.Providers.Migrations/Migrations/M0085_WidenAddressColumns.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0084_WidenAutofillDataColumnPg.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0085_WidenAddressColumnsPg.csProviders/Resgrid.Providers.Number/TextMessageProvider.csProviders/Resgrid.Providers.Workflow/Executors/TwilioSmsExecutor.csRepositories/Resgrid.Repositories.DataRepository/HealthRepository.csWeb/Resgrid.Web/Areas/User/Controllers/HomeController.csWeb/Resgrid.Web/Areas/User/Models/Connect/ProfileView.csWeb/Resgrid.Web/Areas/User/Models/Contacts/AddContactView.csWeb/Resgrid.Web/Areas/User/Models/Contacts/EditContactView.csWeb/Resgrid.Web/Areas/User/Models/DepartmentSettingsModel.csWeb/Resgrid.Web/Areas/User/Models/EditProfileModel.csWeb/Resgrid.Web/Areas/User/Models/Groups/EditGroupView.csWeb/Resgrid.Web/Areas/User/Models/Groups/NewGroupView.csWeb/Resgrid.Web/Areas/User/Views/Connect/Profile.cshtmlWeb/Resgrid.Web/Areas/User/Views/Contacts/Add.cshtmlWeb/Resgrid.Web/Areas/User/Views/Contacts/Edit.cshtmlWeb/Resgrid.Web/Areas/User/Views/Contacts/Index.cshtmlWeb/Resgrid.Web/Areas/User/Views/Department/Address.cshtmlWeb/Resgrid.Web/Areas/User/Views/Department/Settings.cshtmlWeb/Resgrid.Web/Areas/User/Views/Groups/EditGroup.cshtmlWeb/Resgrid.Web/Areas/User/Views/Groups/NewGroup.cshtmlWeb/Resgrid.Web/Areas/User/Views/Home/EditUserProfile.cshtmlWeb/Resgrid.Web/Areas/User/Views/Personnel/AddPerson.cshtmlWeb/Resgrid.Web/wwwroot/js/app/resgrid.phonevalidate.js
✅ Files skipped from review due to trivial changes (3)
- Providers/Resgrid.Providers.Migrations/Migrations/M0084_WidenAutofillDataColumn.cs
- Providers/Resgrid.Providers.MigrationsPg/Migrations/M0084_WidenAutofillDataColumnPg.cs
- Providers/Resgrid.Providers.MigrationsPg/Migrations/M0085_WidenAddressColumnsPg.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Core/Resgrid.Services/CheckInTimerService.cs
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| /// </summary> | ||
| [HttpPost] | ||
| [Authorize(Policy = ResgridResources.Department_View)] | ||
| public IActionResult ValidatePhoneNumber([FromBody] ValidatePhoneNumberRequest request) |
Resgrid-Bot
approved these changes
Jun 26, 2026
Code Review Could Not Complete
|
| Options | Enabled |
|---|---|
| Bug | ✅ |
| Performance | ✅ |
| Security | ✅ |
| Business Logic | ✅ |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Web/Resgrid.Web/Areas/User/Controllers/PersonnelController.cs (1)
490-520: 🎯 Functional Correctness | 🟠 MajorUse a UK region hint or align the carrier checks with normalized input.
ValidateAndNormalize(..., null)falls back to US parsing, so local UK numbers like07911...can be rejected here. The carrier prefix checks also only accept raw0.../44...input, so valid+44...numbers are blocked before normalization.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/Areas/User/Controllers/PersonnelController.cs` around lines 490 - 520, The phone validation in PersonnelController’s profile save flow is using a US default region and raw prefix checks, which blocks valid UK numbers before normalization. Update the MobileNumber validation path to pass a UK region hint into PhoneValidationHelper.ValidateAndNormalize (or otherwise align it with the intended locale), and make the carrier-specific checks in the same block accept normalized/plus-prefixed inputs such as +44 as well as local 0-prefixed forms. Keep the logic centered in the PersonnelController save method and the PhoneValidationHelper call site so the validation and normalization stay consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Web/Resgrid.Web/Areas/User/Controllers/PersonnelController.cs`:
- Around line 490-520: The phone validation in PersonnelController’s profile
save flow is using a US default region and raw prefix checks, which blocks valid
UK numbers before normalization. Update the MobileNumber validation path to pass
a UK region hint into PhoneValidationHelper.ValidateAndNormalize (or otherwise
align it with the intended locale), and make the carrier-specific checks in the
same block accept normalized/plus-prefixed inputs such as +44 as well as local
0-prefixed forms. Keep the logic centered in the PersonnelController save method
and the PhoneValidationHelper call site so the validation and normalization stay
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4651e017-6586-490d-a756-f839bcfc7b99
📒 Files selected for processing (7)
Providers/Resgrid.Providers.Migrations/Migrations/M0085_WidenAddressColumns.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0085_WidenAddressColumnsPg.csRepositories/Resgrid.Repositories.DataRepository/HealthRepository.csWeb/Resgrid.Web/Areas/User/Controllers/ContactsController.csWeb/Resgrid.Web/Areas/User/Controllers/PersonnelController.csWeb/Resgrid.Web/Areas/User/Views/Personnel/AddPerson.cshtmlWeb/Resgrid.Web/Helpers/PhoneValidationHelper.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- Web/Resgrid.Web/Areas/User/Views/Personnel/AddPerson.cshtml
- Repositories/Resgrid.Repositories.DataRepository/HealthRepository.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Description
This PR delivers the backend foundation for offline-first Incident Command (IC) plus several SMS deliverability, phone-number validation, and data-truncation fixes.
1. Offline-First Sync Foundation (RIC-T39)
Establishes the server-side primitives the mobile apps need to work fully offline and reconcile on reconnect (documented in the new
docs/architecture/offline-first-architecture.md):ModifiedOncursor (and aDeletedOnsoft-delete tombstone for lanes) to all mutable incident-command entities via the newIChangeTrackedinterface. Every insert/update stamps it through the newTouch()/UpsertOwnedAsync()helpers.GET /api/v4/Sync/Changes?since=returns every changed row (including soft-deleted/closed/released) for the caller's department, with aServerTimestampMscursor for the next pull.CheckInRecord.IdempotencyKey(with a filtered unique index) dedups replayed offline check-ins; a post-violation recovery adopts the winning row instead of erroring.DeleteNodeAsyncnow sets a tombstone instead of hard-deleting, so removals propagate on delta sync.2. SMS Deliverability & Cost
New
SmsContentHelperapplied at the single chokepoint (and the workflow SMS executor) before sending:SmsMaxLengthandSmsAllowedUrlDomainssettings.3. Phone-Number Validation & E.164 Normalization
PhoneRegionHelpermaps country names to ISO region codes so national-format numbers can be recognized.ContactVerificationServicenow validates/normalizes to E.164 before sending SMS or voice calls (prevents Twilio "Invalid 'To'" failures).ValidatePhoneNumberendpoint and a shared client-sideresgrid.phonevalidate.jsthat offers a one-click canonical fix on the profile, contact, and personnel forms.4. Data-Truncation Fixes
Addressescolumns (street to max, others generously) and tightens model-levelStringLengthvalidation across all address-entry view models so client + server reject oversized values before save. M0085 (both DBs).nvarchar(255)to max to prevent truncation of long call-note templates. M0084 (both DBs).5. Other Fixes
CURRENT_TIMESTAMPso the health check works on PostgreSQL-backed datacenters (was hardcoded to SQL Server +GETDATE()).Model.Contacts[i]→Model.ContactCategories[i]).SaveOrUpdateAsyncto explicitInsertAsync(pre-set GUIDs would otherwise cause silent 0-row updates).Tests
Adds comprehensive coverage for idempotent check-ins (incl. concurrent-replay race), offline-aware IC service behavior (change tracking, idempotent creates, soft-delete, delta pull), ad-hoc resource idempotency/delta,
SmsContentHelper, andPhoneRegionHelper.Summary by CodeRabbit
New Features
Bug Fixes